Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ORM vs migration files inconsistencies #44221

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

ephraimbuddy
Copy link
Contributor

There have been some inconsistences between ORM and migration files but it doesn't fail in tests. This is an attempt to fix the inconsistency and also have it fail in tests

@ephraimbuddy ephraimbuddy force-pushed the investigate-migrations branch 5 times, most recently from 031bfd8 to 4ac20ad Compare November 22, 2024 11:13
@ephraimbuddy ephraimbuddy marked this pull request as ready for review November 22, 2024 11:14
local_cols=["asset_id"],
remote_cols=["id"],
ondelete="CASCADE",
)
Copy link
Contributor Author

@ephraimbuddy ephraimbuddy Nov 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved this to a new file because, for some reason, SQLite could not create this FK in this file. I suspect it is because the asset table was renamed in this migration

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah last I had tried I ran into:

Performing upgrade to the metadata database sqlite:////root/airflow/sqlite/airflow.db
[2024-10-27T22:43:35.430+0000] {migration.py:215} INFO - Context impl SQLiteImpl.
[2024-10-27T22:43:35.431+0000] {migration.py:218} INFO - Will assume non-transactional DDL.
[2024-10-27T22:43:35.432+0000] {migration.py:215} INFO - Context impl SQLiteImpl.
[2024-10-27T22:43:35.433+0000] {migration.py:218} INFO - Will assume non-transactional DDL.
[2024-10-27T22:43:35.433+0000] {db.py:1171} INFO - Creating tables
INFO [alembic.runtime.migration] Context impl SQLiteImpl.
INFO [alembic.runtime.migration] Will assume non-transactional DDL.
INFO [alembic.runtime.migration] Running upgrade 22ed7efa9da2 -> 044f740568ec, Drop ab_user.id foreign key.
INFO [alembic.runtime.migration] Running upgrade 044f740568ec -> d0f1c55954fa, Remove SubDAGs: `is_subdag` & `root_dag_id` columns from DAG table.
INFO [alembic.runtime.migration] Running upgrade d0f1c55954fa -> 0bfc26bc256e, Rename DagModel schedule_interval to timetable_summary.
INFO [alembic.runtime.migration] Running upgrade 0bfc26bc256e -> a2c32e6c7729, Add triggered_by field to DagRun.
INFO [alembic.runtime.migration] Running upgrade a2c32e6c7729 -> 1cdc775ca98f, Drop `execution_date` unique constraint on DagRun.
INFO [alembic.runtime.migration] Running upgrade 1cdc775ca98f -> 522625f6d606, Add tables for backfill.
INFO [alembic.runtime.migration] Running upgrade 522625f6d606 -> 16cbcb1c8c36, Remove redundant index.
INFO [alembic.runtime.migration] Running upgrade 16cbcb1c8c36 -> 44eabb1904b4, Update dag_run_note.user_id and task_instance_note.user_id columns to String.
INFO [alembic.runtime.migration] Running upgrade 44eabb1904b4 -> 0d9e73a75ee4, Add name and group fields to DatasetModel.
INFO [alembic.runtime.migration] Running upgrade 0d9e73a75ee4 -> c3389cd7793f, Add backfill to dag run model.
INFO [alembic.runtime.migration] Running upgrade c3389cd7793f -> 5a5d66100783, Add AssetActive to track orphaning instead of a flag.
INFO [alembic.runtime.migration] Running upgrade 5a5d66100783 -> fb2d4922cd79, Tweak AssetAliasModel to match AssetModel after AIP-76.
INFO [alembic.runtime.migration] Running upgrade fb2d4922cd79 -> 3a8972ecb8f9, Add exception_reason and logical_date to BackfillDagRun.
INFO [alembic.runtime.migration] Running upgrade 3a8972ecb8f9 -> 05234396c6fc, Rename dataset as asset.
/usr/local/lib/python3.9/contextlib.py:126 SAWarning: WARNING: SQL-parsed foreign key constraint '('alias_id', 'asset_alias', 'id')' could not be located in PRAGMA foreign_keys for table asset_alias_asset
/usr/local/lib/python3.9/contextlib.py:126 SAWarning: WARNING: SQL-parsed foreign key constraint '('dataset_id', 'asset', 'id')' could not be located in PRAGMA foreign_keys for table asset_alias_asset
Traceback (most recent call last):
File "/usr/local/lib/python3.9/site-packages/alembic/operations/batch.py", line 690, in drop_constraint
const = self.named_constraints.pop(const.name)
KeyError: 'dataset_alias_dataset_alias_id_fkey'

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warnings:

/usr/local/lib/python3.9/contextlib.py:126 SAWarning: WARNING: SQL-parsed foreign key constraint '('alias_id', 'asset_alias', 'id')' could not be located in PRAGMA foreign_keys for table asset_alias_asset
/usr/local/lib/python3.9/contextlib.py:126 SAWarning: WARNING: SQL-parsed foreign key constraint '('dataset_id', 'asset', 'id')' could not be located in PRAGMA foreign_keys for table asset_alias_asset
/usr/local/lib/python3.9/contextlib.py:126 SAWarning: WARNING: SQL-parsed foreign key constraint '('alias_id', 'asset_alias', 'id')' could not be located in PRAGMA foreign_keys for table asset_alias_asset_event
/usr/local/lib/python3.9/contextlib.py:126 SAWarning: WARNING: SQL-parsed foreign key constraint '('event_id', 'asset_event', 'id')' could not be located in PRAGMA foreign_keys for table asset_alias_asset_event

@ephraimbuddy ephraimbuddy added the full tests needed We need to run full set of tests for this PR to merge label Nov 22, 2024
@ephraimbuddy ephraimbuddy reopened this Nov 22, 2024
local_cols=["asset_id"],
remote_cols=["id"],
ondelete="CASCADE",
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah last I had tried I ran into:

Performing upgrade to the metadata database sqlite:////root/airflow/sqlite/airflow.db
[2024-10-27T22:43:35.430+0000] {migration.py:215} INFO - Context impl SQLiteImpl.
[2024-10-27T22:43:35.431+0000] {migration.py:218} INFO - Will assume non-transactional DDL.
[2024-10-27T22:43:35.432+0000] {migration.py:215} INFO - Context impl SQLiteImpl.
[2024-10-27T22:43:35.433+0000] {migration.py:218} INFO - Will assume non-transactional DDL.
[2024-10-27T22:43:35.433+0000] {db.py:1171} INFO - Creating tables
INFO [alembic.runtime.migration] Context impl SQLiteImpl.
INFO [alembic.runtime.migration] Will assume non-transactional DDL.
INFO [alembic.runtime.migration] Running upgrade 22ed7efa9da2 -> 044f740568ec, Drop ab_user.id foreign key.
INFO [alembic.runtime.migration] Running upgrade 044f740568ec -> d0f1c55954fa, Remove SubDAGs: `is_subdag` & `root_dag_id` columns from DAG table.
INFO [alembic.runtime.migration] Running upgrade d0f1c55954fa -> 0bfc26bc256e, Rename DagModel schedule_interval to timetable_summary.
INFO [alembic.runtime.migration] Running upgrade 0bfc26bc256e -> a2c32e6c7729, Add triggered_by field to DagRun.
INFO [alembic.runtime.migration] Running upgrade a2c32e6c7729 -> 1cdc775ca98f, Drop `execution_date` unique constraint on DagRun.
INFO [alembic.runtime.migration] Running upgrade 1cdc775ca98f -> 522625f6d606, Add tables for backfill.
INFO [alembic.runtime.migration] Running upgrade 522625f6d606 -> 16cbcb1c8c36, Remove redundant index.
INFO [alembic.runtime.migration] Running upgrade 16cbcb1c8c36 -> 44eabb1904b4, Update dag_run_note.user_id and task_instance_note.user_id columns to String.
INFO [alembic.runtime.migration] Running upgrade 44eabb1904b4 -> 0d9e73a75ee4, Add name and group fields to DatasetModel.
INFO [alembic.runtime.migration] Running upgrade 0d9e73a75ee4 -> c3389cd7793f, Add backfill to dag run model.
INFO [alembic.runtime.migration] Running upgrade c3389cd7793f -> 5a5d66100783, Add AssetActive to track orphaning instead of a flag.
INFO [alembic.runtime.migration] Running upgrade 5a5d66100783 -> fb2d4922cd79, Tweak AssetAliasModel to match AssetModel after AIP-76.
INFO [alembic.runtime.migration] Running upgrade fb2d4922cd79 -> 3a8972ecb8f9, Add exception_reason and logical_date to BackfillDagRun.
INFO [alembic.runtime.migration] Running upgrade 3a8972ecb8f9 -> 05234396c6fc, Rename dataset as asset.
/usr/local/lib/python3.9/contextlib.py:126 SAWarning: WARNING: SQL-parsed foreign key constraint '('alias_id', 'asset_alias', 'id')' could not be located in PRAGMA foreign_keys for table asset_alias_asset
/usr/local/lib/python3.9/contextlib.py:126 SAWarning: WARNING: SQL-parsed foreign key constraint '('dataset_id', 'asset', 'id')' could not be located in PRAGMA foreign_keys for table asset_alias_asset
Traceback (most recent call last):
File "/usr/local/lib/python3.9/site-packages/alembic/operations/batch.py", line 690, in drop_constraint
const = self.named_constraints.pop(const.name)
KeyError: 'dataset_alias_dataset_alias_id_fkey'

local_cols=["asset_id"],
remote_cols=["id"],
ondelete="CASCADE",
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warnings:

/usr/local/lib/python3.9/contextlib.py:126 SAWarning: WARNING: SQL-parsed foreign key constraint '('alias_id', 'asset_alias', 'id')' could not be located in PRAGMA foreign_keys for table asset_alias_asset
/usr/local/lib/python3.9/contextlib.py:126 SAWarning: WARNING: SQL-parsed foreign key constraint '('dataset_id', 'asset', 'id')' could not be located in PRAGMA foreign_keys for table asset_alias_asset
/usr/local/lib/python3.9/contextlib.py:126 SAWarning: WARNING: SQL-parsed foreign key constraint '('alias_id', 'asset_alias', 'id')' could not be located in PRAGMA foreign_keys for table asset_alias_asset_event
/usr/local/lib/python3.9/contextlib.py:126 SAWarning: WARNING: SQL-parsed foreign key constraint '('event_id', 'asset_event', 'id')' could not be located in PRAGMA foreign_keys for table asset_alias_asset_event

There have been some inconsistences between ORM and migration files
but it doesn't fail in tests. This is an attempt to fix the inconsistency
and also have it fail in tests
@ephraimbuddy ephraimbuddy self-assigned this Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:db-migrations PRs with DB migration area:dev-tools full tests needed We need to run full set of tests for this PR to merge kind:documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants